Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Transform String Resugar Term Scott #714

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

Liberxue
Copy link
Contributor

@Liberxue Liberxue commented Sep 11, 2024

#713

  • Create struct to wrap user-provided cons handling functions, implementing the trait.
  • Implement struct to handle default cons patterns, also implementing the trait.
  • Replace with type, better expressing success or failure semantics.
  • Utilize Rust's pattern matching and method to simplify error handling process.

…ndling

* Introduce  trait, allowing definition of visitors for different Term processing logics.
* Create  struct to wrap user-provided cons handling functions, implementing the  trait.
* Implement  struct to handle default cons patterns, also implementing the  trait.
* Replace  with  type, better expressing success or failure semantics.
* Utilize Rust's pattern matching and  method to simplify error handling process.
Copy link
Member

@developedby developedby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visitor is completely unnecessary here, doesn't really simplify.

I like the idea of the try_resugar functions, it lets the pattern matching not be repeated twice.

You could have a single build_string function that takes takes the encoding, tries the correct encoded pattern and tries the non-applied constructors, without repetition.

Also, please leave the if tree since it mimics the shape of the AST and is easier for me to visualize the structure.

@Liberxue
Copy link
Contributor Author

The visitor is completely unnecessary here, doesn't really simplify.

I like the idea of the try_resugar functions, it lets the pattern matching not be repeated twice.

You could have a single build_string function that takes takes the encoding, tries the correct encoded pattern and tries the non-applied constructors, without repetition.

Also, please leave the if tree since it mimics the shape of the AST and is easier for me to visualize the structure.

Done Please review~~thx @developedby

@Liberxue Liberxue changed the title Refactor Transform String Resugar Term Scott with visitor pattern and improved error handling Refactor Transform String Resugar Term Scott Sep 11, 2024
Copy link
Member

@developedby developedby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nicer to read without the visitor, but my other previous comments still apply.

Also, please do the changes on resugar_list as well. It's almost an exact copy of this one, just looking for a slightly different pattern. It would be confusing if they were implemented differently

}

// Cons: @x (x CONS_TAG <num> <str>)
if let Term::Lam { tag: Tag::Static, pat, bod } = current {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specifically the "num-scott" encoding, you shouldn't apply it for all cases

@Liberxue
Copy link
Contributor Author

This is nicer to read without the visitor, but my other previous comments still apply.

Also, please do the changes on resugar_list as well. It's almost an exact copy of this one, just looking for a slightly different pattern. It would be confusing if they were implemented differently

In the resugar_lists file, after careful reading, I found that extracting code blocks could lead to reduced performance due to cloning. I haven't thought of a better approach at the moment~~~

…ics the shape of the AST for easier visualization
…ics the shape of the AST for easier visualization
@developedby
Copy link
Member

Can you just run cargo fmt?

… the shape of the AST visualize the structure && cargo fmt
@Liberxue
Copy link
Contributor Author

Can you just run cargo fmt?
So Sorry OKK~~ done

@developedby developedby added this pull request to the merge queue Sep 17, 2024
@developedby
Copy link
Member

Thank you very much for the contribution

Merged via the queue into HigherOrderCO:main with commit 2b401e7 Sep 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants